prov/efa: Fix FI_INJECT in efa direct#12146
Merged
Merged
Conversation
sunkuamzn
requested changes
Apr 15, 2026
Contributor
|
Does inject + write handle this correctly? |
Contributor
Author
Inline write is implemented as part of wide wqe #11944 |
…_initialization The unit test binary is a single persistent process — all tests share the same efa_device with its qp_gen_table. Every test that creates an endpoint with data path direct enabled increments qp_gen_table[qpn_slot]. gen is a uint8_t that increments on every QP creation for that QPN slot across the entire test process. It will inevitably wrap to 0 after 256 QP cycles. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>
jiaxiyan
changed the title
sunkuamzn
reviewed
Apr 16, 2026
shijin-aws
reviewed
Apr 16, 2026
shijin-aws
reviewed
Apr 16, 2026
| "FI_INJECT is requested but message size of " | ||
| "%zu exceeds efa-direct inject_msg_size of " | ||
| "%zu, " | ||
| "or desc is on device memory: %d.\n", |
Contributor
There was a problem hiding this comment.
Do u think two warning messages based on two if would be better? like
if (len > base_ep->domain->device->efa_attr.inline_buf_size)
EFA_WARN("msg size exceeds inject size")
if (msg->desc && efa_mr_is_hmem(msg->desc[0]))
EFA_WARN("inject/FI_INJECT is not supported for FI_HMEM memory\n")
Contributor
There was a problem hiding this comment.
Help the user out; tell them which thing was wrong.
shijin-aws
previously approved these changes
Apr 16, 2026
sunkuamzn
reviewed
Apr 16, 2026
| } | ||
| } else { | ||
| if (flags & FI_INJECT) { | ||
| if (len > base_ep->inject_msg_size) |
Contributor
There was a problem hiding this comment.
We're duplicating the check
Can we do the check once and handle both use_inline and flags & FI_INJECT?
Contributor
Author
There was a problem hiding this comment.
I don't follow. What is your suggested change?
Contributor
There was a problem hiding this comment.
Something like
bool len_less_than_inline = len <= base_ep->domain->device->efa_attr.inline_buf_size;
bool is_hmem = msg->desc && efa_mr_is_hmem(msg->desc[0]);
if (len_less_than_inline && !is_hmem) {
use_inline = true;
} else {
if (flags & FI_INJECT) {
if (len_less_than_inline) // print error for length
else // print error for hmem
}
}
Return -FI_EOPNOTSUPP for FI_INJECT when EFA device cannot use inline send. Replace the assert in fi_inject/fi_injectdata, fail with -FI_EOPNOTSUPP instead. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>
shijin-aws
approved these changes
Apr 17, 2026
sunkuamzn
approved these changes
Apr 17, 2026
darrylabbate
added a commit
to darrylabbate/libfabric
that referenced
this pull request
Apr 29, 2026
…d_prep The post_send mock queued via will_return_int_always is not consumed by tests that override g_efa_unit_test_mocks.efa_qp_post_send with a different mock. cmocka then reports the queued value as a leak and fails the test with: [ ERROR ] --- %s() has remaining non-returned values. prov/efa/test/efa_unit_test_msg.c: remaining item was declared here Use will_return_int_maybe instead so the queued value may go unused when a test installs its own post_send mock. This is a targeted extract of upstream commit a2f162a (ofiwg#12146, "prov/efa: Fix FI_INJECT in efa direct"), limited to the single will_return_int_maybe change needed as a prerequisite for the test_efa_msg_sendmsg_multi_iov_second_desc_hmem_fails test added by the v2.4.x backport of ofiwg#12163. Signed-off-by: Darryl Abbate <drl@amazon.com>
darrylabbate
added a commit
that referenced
this pull request
Apr 29, 2026
…d_prep The post_send mock queued via will_return_int_always is not consumed by tests that override g_efa_unit_test_mocks.efa_qp_post_send with a different mock. cmocka then reports the queued value as a leak and fails the test with: [ ERROR ] --- %s() has remaining non-returned values. prov/efa/test/efa_unit_test_msg.c: remaining item was declared here Use will_return_int_maybe instead so the queued value may go unused when a test installs its own post_send mock. This is a targeted extract of upstream commit a2f162a (#12146, "prov/efa: Fix FI_INJECT in efa direct"), limited to the single will_return_int_maybe change needed as a prerequisite for the test_efa_msg_sendmsg_multi_iov_second_desc_hmem_fails test added by the v2.4.x backport of #12163. Signed-off-by: Darryl Abbate <drl@amazon.com>
darrylabbate
added a commit
to darrylabbate/libfabric
that referenced
this pull request
May 6, 2026
…d_prep The post_send mock queued via will_return_int_always is not consumed by tests that override g_efa_unit_test_mocks.efa_qp_post_send with a different mock. cmocka then reports the queued value as a leak and fails the test with: [ ERROR ] --- %s() has remaining non-returned values. prov/efa/test/efa_unit_test_msg.c: remaining item was declared here Use will_return_int_maybe instead so the queued value may go unused when a test installs its own post_send mock. This is a targeted extract of upstream commit a2f162a (ofiwg#12146, "prov/efa: Fix FI_INJECT in efa direct"), limited to the single will_return_int_maybe change needed as a prerequisite for the test_efa_msg_sendmsg_multi_iov_second_desc_hmem_fails test added by the v2.5.x backport of ofiwg#12163. Signed-off-by: Darryl Abbate <drl@amazon.com>
shijin-aws
pushed a commit
that referenced
this pull request
May 7, 2026
…d_prep The post_send mock queued via will_return_int_always is not consumed by tests that override g_efa_unit_test_mocks.efa_qp_post_send with a different mock. cmocka then reports the queued value as a leak and fails the test with: [ ERROR ] --- %s() has remaining non-returned values. prov/efa/test/efa_unit_test_msg.c: remaining item was declared here Use will_return_int_maybe instead so the queued value may go unused when a test installs its own post_send mock. This is a targeted extract of upstream commit a2f162a (#12146, "prov/efa: Fix FI_INJECT in efa direct"), limited to the single will_return_int_maybe change needed as a prerequisite for the test_efa_msg_sendmsg_multi_iov_second_desc_hmem_fails test added by the v2.5.x backport of #12163. Signed-off-by: Darryl Abbate <drl@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Return -FI_EOPNOTSUPP for FI_INJECT when EFA device cannot use inline send.
Replace the assert in fi_inject/fi_injectdata. Fail with -FI_EOPNOTSUPP instead.